feat: resource operation support multiple update modes#3491
Conversation
b978daa to
f0d3fcb
Compare
There was a problem hiding this comment.
Pull request overview
This PR reworks ResourceOperations to support multiple update modes (match-before-write, optimistic-locking-aware filtering, cache-only, force-filter), introduces a new Matcher SPI plus default matchers per update type, and expands test/doc coverage around own-event filtering correctness and read-cache-after-write behavior.
Changes:
- Adds matcher-aware update/patch/create modes to
ResourceOperations(including default matchers per operation type). - Introduces new matcher implementations (including status-only matching) and expands unit/integration tests for own-event filtering edge cases.
- Updates documentation/migration notes for v5.5 and adjusts CI integration-test timeout.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/subresource/SubResourceUpdateIT.java | Updates assertions to reflect additional reconciliation triggered by status updates. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/SecondaryResourceOperationsReconciler.java | New reconciler exercising secondary-resource ResourceOperations overloads and update modes. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/SecondaryResourceOperationsIT.java | New IT covering secondary-resource operations convergence without looping. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/SecondaryResourceOperationsCustomResource.java | New CR type for secondary resource operations tests. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsStatus.java | New status model for ResourceOperations test CRs. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsSpec.java | New spec model for ResourceOperations test CRs. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsReconciler.java | New reconciler exercising primary update/patch/SSA + status variants via ResourceOperations. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsIT.java | New IT covering all primary update/patch/SSA strategies and convergence. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsCustomResource.java | New CR type for primary resource operations tests. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchStatus.java | New status type for spec-change-during-status-patch reproduction test. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchSpec.java | New spec type for spec-change-during-status-patch reproduction test. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchReconciler.java | New reconciler reproducing concurrent spec change during status patch filtering window. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchIT.java | New repeated IT validating spec change is not swallowed by own status patch filtering. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/specchangeduringstatuspatch/SpecChangeDuringStatusPatchCustomResource.java | New CR type for the spec-change-during-status-patch scenario. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownssastatusupdate/OwnSsaStatusUpdateStatus.java | New status type for own SSA status update scenario. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownssastatusupdate/OwnSsaStatusUpdateReconciler.java | New reconciler applying status via resourceOperations().serverSideApplyPrimaryStatus(...). |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownssastatusupdate/OwnSsaStatusUpdateIT.java | New IT asserting own SSA status update doesn’t loop and external updates still reconcile. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownssastatusupdate/OwnSsaStatusUpdateCustomResource.java | New CR type for own SSA status update test. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/ownsecondaryupdate/OwnSecondaryUpdateReconciler.java | Switches SSA calls to explicit force-filter options for secondary own-event filtering. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/readcacheafterwrite/onrelistfilter/OnRelistFilterReconciler.java | Updates SSA calls to force-filter options for relist filtering scenarios. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/changenamespace/ChangeNamespaceTestReconciler.java | Updates SSA call to force-filter options. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/matcher/StatusMatchersTest.java | New unit tests for status-only matchers behavior. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/matcher/PatchMatchersTest.java | New unit tests for JSON patch/merge patch matchers (resource + status). |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java | Adds test ensuring force-filter opens window even without resourceVersion. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java | Adds tests for new matchStatus(...) behavior and equality modes. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java | Extends tests for matcher-driven patching, cache-only, and filtering modes. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/UpdateType.java | New enum mapping update types to default matchers. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/UpdateStatusMatcher.java | New matcher for update-status operations using status-only matching. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/UpdateMatcher.java | New matcher for update operations using generic resource matching. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/SSAStatusMatcher.java | New matcher for SSA status operations using status-only matching. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/SSAMatcher.java | New matcher for SSA operations using SSA-based matcher implementation. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/MatcherUtils.java | New shared utilities for JSON patch/merge patch matcher implementations. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/JsonPatchStatusMacher.java | New JSON Patch status matcher implementation. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/JsonPatchMacher.java | New JSON Patch matcher implementation. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/JsonMergePatchStatusMatcher.java | New JSON Merge Patch status matcher implementation. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/matcher/JsonMergePatchMatcher.java | New JSON Merge Patch matcher implementation. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java | Adds update-and-cache helper and annotates update paths as experimental. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java | Adjusts primary patch paths to use cache-only ResourceOperations variants. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java | Updates dependent resource create/update to use explicit force-filter options. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java | Adds matchStatus(...) to compare only /status subtree with configurable equality. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java | Major API expansion: modes/options, matchers, default matcher selection per operation type. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/matcher/Matcher.java | New matcher SPI for plug-in match logic. |
| docs/content/en/docs/migration/v5-5-migration.md | New migration doc describing v5.5 behavioral change around UpdateControl. |
| docs/content/en/docs/documentation/reconciler.md | Documents new ResourceOperations.Options modes and UpdateControl behavior change. |
| docs/content/en/blog/releases/v5-5-release.md | New release post for v5.5 summarizing features and migration notes. |
| docs/content/en/blog/news/read-after-write-consistency.md | Updates blog content to reflect explicit facilities for own-event filtering. |
| .github/workflows/integration-tests.yml | Increases integration test job timeout. |
440b380 to
2d7c490
Compare
| // UpdateControl.patchStatus would only cache the resource to filter out events too | ||
| // you have to use resourceOperations. |
There was a problem hiding this comment.
this should be better structured, it can be confusing
| @@ -0,0 +1,120 @@ | |||
| --- | |||
| title: Version 5.5 Released! | |||
There was a problem hiding this comment.
Shouldn't this be a separate PR? Won't you squash commits on merge?
There was a problem hiding this comment.
Yes, but since this is I have only this PR related changes in here, I added as a part of the commit, want to make a separate PR for all other changes after we merged this. But if you insist can have this as a separate PR.
There was a problem hiding this comment.
for history purposes it would be better to split
There was a problem hiding this comment.
changed! created a separate PR: #3497
Thank you!
| available for the whole resource and the `status` subresource, and with dedicated `primary` | ||
| variants. | ||
|
|
||
| Every method comes in two flavors: a default one, and one taking an `Options` argument that controls |
There was a problem hiding this comment.
Maybe we should also have a global default change so the user doesn't have to pass the argument to each method call.
There was a problem hiding this comment.
Not sure what do you mean, could you pls elaborate ? maybe with some code snippet
| public <R extends HasMetadata> R create(R resource) { | ||
| return resourcePatch(resource, r -> context.getClient().resource(r).create()); | ||
| // it is safe to do event filtering for create since check if the resource already exists. | ||
| return create(resource, Options.forceFilterEvents()); |
There was a problem hiding this comment.
I understand, however, once something is default, I think it should be default everywhere. If you implement that changing of default as I suggested above, this might be confusing. Maybe then don't provide Options parametrized overload and just keep this as implementation detail.
There was a problem hiding this comment.
The default methods are always filtering, we achieve that with the Matchers (to cover the problematic edge case). We however want to give the user possibility to not filter (cache-only). The API might be more restrictive in that regard, but this way it is also forward compatible.
| boolean matched = true; | ||
| for (int i = 0; i < wholeDiffJsonPatch.size() && matched; i++) { | ||
| var node = wholeDiffJsonPatch.get(i); | ||
| if (nodeIsChildOf(node, List.of(STATUS))) { |
There was a problem hiding this comment.
should this also support nested values?
There was a problem hiding this comment.
it supports nested values, there is a recursive call there.
There was a problem hiding this comment.
ok, didn't check, thanks
723c493 to
7fe336d
Compare
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
The status assertions could throw NPE on the first Awaitility poll before the reconciler patched the status. untilAsserted retries AssertionErrors but rethrows other exceptions immediately, so the NPE aborted the test instead of retrying. Guard with assertThat(status).isNotNull() to keep polling. Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Co-authored-by: Martin Stefanko <xstefank122@gmail.com> Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
…e-blog) Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
fd96006 to
35d020e
Compare
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Rework ResourceOperations into the primary API for create/update/patch calls, giving explicit control over read-after-write cache consistency and own-event filtering via an Options/Mode strategy plus a pluggable Matcher framework. Own-event filtering was previously implicit and could silently swallow concurrent third-party writes. Options now make the tradeoff explicit per call: - matchAndFilter / matchAndFilterWithDefaultMatcher (default): compare desired vs. cached state, skip the write if it already matches, otherwise write and filter the own event. - filterWithOptimisticLocking: filter the own event but require optimistic locking so concurrent changes are rejected server-side instead of silently dropped. - cacheOnly: update the cache only, no event filtering. - forceFilterEvents: always filter (internal use; requires correctness guaranteed elsewhere). Add a Matcher framework with default matchers per UpdateType (UPDATE, SSA, JSON_PATCH, JSON_MERGE_PATCH, and their *_STATUS variants), backed by MatcherUtils, plus GenericKubernetesResourceMatcher.matchStatus for status-subtree comparisons. Framework integration: - ReconciliationDispatcher adds the finalizer with cacheOnly instead of filter + INSTANT_RESCHEDULE, so the finalizer-add event itself drives the next reconcile. - Primary UpdateControl writes use cacheOnly, trading an extra self-triggered reconcile for correctness (a concurrent spec change during a status patch can no longer be silently absorbed). - KubernetesDependentResource create/SSA use forceFilterEvents. - AbstractExternalDependentResource persists state through resourceOperations().create(...). Add ITs covering the concurrency and read-after-write edge cases (ResourceOperationsIT, SecondaryResourceOperationsIT, SpecChangeDuringStatusPatchIT, OwnSsaStatusUpdateIT) and update docs (reconciler.md, v5-5-migration.md, v5-5-release.md, read-after-write blog post). The Options API is marked @experimental; default matchers are heuristics, so reconcilers should test them against concrete resources or supply a custom Matcher. Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Summary
This reworks
ResourceOperationsinto the primary, idiomatic API for create/update/patch operations that keep the informer cache read-after-write consistent and control how the resulting own event is handled. Every operation comes in adefault flavor and an
Options-taking flavor, so users can tune caching vs. own-event filtering per call.Own-event filtering is only correct when the framework can distinguish an own write from a concurrent third-party write. This PR makes that requirement explicit through a small strategy model (
Options→Mode) and a pluggableMatcherframework with sensible per-operation defaults.
Options/ModematchAndFilter(matcher)/matchAndFilterWithDefaultMatcher(updateType)— compare desired vs. actual (cached) state; skip the write entirely if they already match, otherwise write and filter the own event. This is the default for theupdate/patch methods and the most efficient option (filters and avoids a needless API call).
filterWithOptimisticLocking()— filter the own event; the write must use optimistic locking (a resourceVersion set), otherwise anIllegalArgumentExceptionis thrown. Guarantees a concurrent change is rejected server-side rather thansilently filtered out.
cacheOnly()— only cache the response (read-after-write consistency), no own-event filtering.forceFilterEvents()— always filter (mostly internal; safe only when correctness is otherwise guaranteed).Matcher framework
Matcherinterface plus default matchers for everyUpdateType(UPDATE,SSA,JSON_PATCH,JSON_MERGE_PATCH, and their*_STATUSvariants), backed byMatcherUtils.GenericKubernetesResourceMatcher.matchStatus(...)added to match only the/statussubtree (tolerating server-added fields by default).Framework integration
ReconciliationDispatcher): the finalizer is now added withcacheOnly(its own event is no longer filtered), replacing the previous filter +INSTANT_RESCHEDULEso the finalizer-add event itself drives the follow-upreconcile.
UpdateControlwrites now usecacheOnly, trading one extra self-triggered reconcile for correctness (a concurrent spec change during a status patch can no longer be absorbed by the own-event filter).KubernetesDependentResourcecreate/SSA now useforceFilterEvents().AbstractExternalDependentResourcepersists explicit state throughresourceOperations().create(...).Tests & docs
ResourceOperationsIT,SecondaryResourceOperationsIT,SpecChangeDuringStatusPatchIT,OwnSsaStatusUpdateIT, plus expandedResourceOperationsTest,PatchMatchersTest,StatusMatchersTest.SubResourceUpdateIT,WorkflowMultipleActivationITnow snapshot reconcile counts only after they stabilize).reconciler.md,v5-5-migration.md,v5-5-release.md, and the read-after-write blog post.Notes / follow-ups
OptionsAPI is marked@Experimental(API may change).Matcher.